[codex] improve mobile workspace UX - #556
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughIntroduces a workspace view-policy module exposing viewport mode ("desktop"/"mobile") and capability flags (context menus, file capture) via React context. Wires capabilities into viewers, item cards, and content browsing to gate context menus and capture UI, adds a mobile layout, an upload click target component, and applies responsive Tailwind styling adjustments. ChangesWorkspace view capabilities and mobile layout
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant Shell as WorkspaceShell
participant Policy as useWorkspaceViewPolicy
participant Mobile as WorkspaceMobileLayout
participant Provider as WorkspaceViewCapabilitiesProvider
participant Viewer as WorkspacePdfViewer/WorkspaceItemCard
Shell->>Policy: read viewportMode and capabilities
Policy-->>Shell: viewportMode, capabilities
alt viewportMode is mobile
Shell->>Mobile: render mobile shell with contextBar, standardTabPanes, chatPanel
else viewportMode is desktop
Shell->>Shell: render docked/maximized presentation
end
Shell->>Provider: wrap presentationContent with capabilities
Provider->>Viewer: expose capabilities via context
Viewer->>Viewer: conditionally render ContextMenu and capture UI
Suggested labels: ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 67088ea060
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| onOpenChat, | ||
| }: WorkspaceMobileLayoutProps) { | ||
| const [shareOpen, setShareOpen] = useState(false); | ||
| const isChatOpen = chatSurfaceMode !== "hidden"; |
There was a problem hiding this comment.
On mobile, this treats every non-hidden mode as an open full-screen overlay, but the normalized default session uses chatSurfaceMode: "docked". As a result, any first visit on a phone (or any user whose desktop state is docked) immediately renders the fixed z-50 chat over the workspace and hides the Chat button, so the new mobile browse UI is covered until the user discovers and closes chat; normalize docked to closed on mobile or only open the overlay for fullscreen.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/features/workspaces/components/workspace-view-policy.tsx`:
- Around line 38-39: The server snapshot in getWorkspaceViewportServerSnapshot()
is hardcoded to "desktop", which causes mobile hydration flicker; update this
function to derive the initial WorkspaceViewportMode from a server-side viewport
hint or use a neutral fallback until the client can determine the real mode.
Keep the fix localized to getWorkspaceViewportServerSnapshot() and the workspace
viewport policy logic so the server-rendered shell matches the eventual client
viewport as closely as possible.
In `@src/features/workspaces/components/WorkspaceContent.tsx`:
- Around line 354-358: The uploadable empty-state wrapper in WorkspaceContent
breaks the flex sizing applied by Empty because WorkspaceUploadClickTarget
becomes the flex child instead of the Empty component itself. Update the
canUpload branch in WorkspaceContent so the wrapped empty state still fills the
browse column, either by moving the flex-growing class/style onto
WorkspaceUploadClickTarget or by wrapping the empty state in a flex container
that preserves the existing flex-1 behavior.
In `@src/features/workspaces/components/WorkspaceFileViewer.tsx`:
- Around line 59-67: The two branches in WorkspaceFileViewer should keep the
same outer viewer shell so layout and overflow behavior stay consistent. Update
the no-context-menu path to use the same h-full min-h-0 overflow-hidden wrapper
as the ContextMenuTrigger path, while preserving the existing viewerContent
placement inside WorkspaceFileViewer. Ensure both the context-menu and
non-context-menu branches share the same shell structure, differing only by the
presence of ContextMenu/ContextMenuTrigger.
In `@src/features/workspaces/components/WorkspaceHomePage.tsx`:
- Around line 90-91: The workspace-list search is currently hidden on mobile
because the search container in WorkspaceHomePage is set to hide below the sm
breakpoint, and WorkspaceMobileLayout does not provide an equivalent control.
Update WorkspaceHomePage so the workspace filter/search remains available on
mobile, or add a mobile replacement in WorkspaceMobileLayout, and make sure the
workspace-list search stays separate from the WorkspaceContextBar search used
inside an open workspace.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 452cacc4-054a-48e7-85cb-589ddd337ddf
📒 Files selected for processing (20)
src/components/AppShell.tsxsrc/features/workspaces/components/WorkspaceContent.tsxsrc/features/workspaces/components/WorkspaceFileToolbar.tsxsrc/features/workspaces/components/WorkspaceFileViewer.tsxsrc/features/workspaces/components/WorkspaceHomePage.tsxsrc/features/workspaces/components/WorkspaceImageViewer.tsxsrc/features/workspaces/components/WorkspaceItemCard.tsxsrc/features/workspaces/components/WorkspaceItemPreviewSurface.tsxsrc/features/workspaces/components/WorkspaceItemToolbarSlot.tsxsrc/features/workspaces/components/WorkspaceLayout.tsxsrc/features/workspaces/components/WorkspaceMobileLayout.tsxsrc/features/workspaces/components/WorkspacePdfViewer.tsxsrc/features/workspaces/components/WorkspaceRootEmptyPreview.tsxsrc/features/workspaces/components/WorkspaceUploadClickTarget.tsxsrc/features/workspaces/components/ai-chat/AiChatPanelToolbar.tsxsrc/features/workspaces/components/workspace-item-card-chrome.tssrc/features/workspaces/components/workspace-item-card-footer.tsxsrc/features/workspaces/components/workspace-item-card-preview-controls.tsxsrc/features/workspaces/components/workspace-item-card-preview-stage.tsxsrc/features/workspaces/components/workspace-view-policy.tsx
| function getWorkspaceViewportServerSnapshot(): WorkspaceViewportMode { | ||
| return "desktop"; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
fd -i 'entry*' .
rg -n --hidden -S 'renderToPipeableStream|renderToReadableStream|hydrateRoot|createRoot' .
rg -n --hidden -S 'WorkspaceShell|WorkspaceLayout' .Repository: ThinkEx-OSS/thinkex
Length of output: 3291
Avoid hardcoding the server viewport snapshot to desktop. getWorkspaceViewportServerSnapshot() always returns "desktop", so hydrated mobile sessions will briefly render the desktop shell before switching to mobile. Seed the server snapshot from a viewport hint or return a neutral shell until the client value is known.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/features/workspaces/components/workspace-view-policy.tsx` around lines 38
- 39, The server snapshot in getWorkspaceViewportServerSnapshot() is hardcoded
to "desktop", which causes mobile hydration flicker; update this function to
derive the initial WorkspaceViewportMode from a server-side viewport hint or use
a neutral fallback until the client can determine the real mode. Keep the fix
localized to getWorkspaceViewportServerSnapshot() and the workspace viewport
policy logic so the server-rendered shell matches the eventual client viewport
as closely as possible.
| <div className="hidden w-full min-w-0 items-center justify-center sm:flex"> | ||
| <div className="relative w-full min-w-0 max-w-72"> |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether the mobile layout exposes an alternate search entry point.
rg -n "search" -i src/features/workspaces/components/WorkspaceMobileLayout.tsx 2>/dev/null
fd -p WorkspaceMobileLayout.tsxRepository: ThinkEx-OSS/thinkex
Length of output: 218
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- outline WorkspaceMobileLayout.tsx ---'
ast-grep outline src/features/workspaces/components/WorkspaceMobileLayout.tsx --view expanded || true
echo '--- related workspace components with search/filter terms ---'
rg -n -i "search|filter" src/features/workspaces/components src/features/workspaces 2>/dev/null || true
echo '--- file size ---'
wc -l src/features/workspaces/components/WorkspaceMobileLayout.tsx || trueRepository: ThinkEx-OSS/thinkex
Length of output: 50375
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,220p' src/features/workspaces/components/WorkspaceMobileLayout.tsx
printf '\n---\n'
rg -n -i "search|filter" src/features/workspaces -g '!**/node_modules/**' || trueRepository: ThinkEx-OSS/thinkex
Length of output: 38658
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- WorkspaceContextBar excerpt ---'
sed -n '60,210p' src/features/workspaces/components/WorkspaceContextBar.tsx
echo '\n--- WorkspaceContextActions excerpt ---'
sed -n '1,120p' src/features/workspaces/components/WorkspaceContextActions.tsxRepository: ThinkEx-OSS/thinkex
Length of output: 6411
Keep workspace-list search available on mobile. src/features/workspaces/components/WorkspaceHomePage.tsx:90-91 hides the only workspace filter below sm, and src/features/workspaces/components/WorkspaceMobileLayout.tsx doesn’t add a replacement. The search action in WorkspaceContextBar is for items inside an open workspace, not the workspace list.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/features/workspaces/components/WorkspaceHomePage.tsx` around lines 90 -
91, The workspace-list search is currently hidden on mobile because the search
container in WorkspaceHomePage is set to hide below the sm breakpoint, and
WorkspaceMobileLayout does not provide an equivalent control. Update
WorkspaceHomePage so the workspace filter/search remains available on mobile, or
add a mobile replacement in WorkspaceMobileLayout, and make sure the
workspace-list search stays separate from the WorkspaceContextBar search used
inside an open workspace.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
7 issues found across 20 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/features/workspaces/components/WorkspaceHomePage.tsx">
<violation number="1" location="src/features/workspaces/components/WorkspaceHomePage.tsx:90">
P2: The workspace list search/filter is completely hidden below the `sm` breakpoint (`hidden ... sm:flex`), and no replacement is provided in the mobile layout. Mobile users lose the ability to search/filter their workspace list entirely.</violation>
</file>
<file name="src/features/workspaces/components/WorkspaceImageViewer.tsx">
<violation number="1" location="src/features/workspaces/components/WorkspaceImageViewer.tsx:57">
P3: Clear capture state when file capture becomes disabled; otherwise resizing back to desktop can unexpectedly restore an old hidden capture session.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| return ( | ||
| <div className="flex w-full min-w-0 items-center justify-center"> | ||
| <div className="relative w-full min-w-0 max-w-56 sm:max-w-72"> | ||
| <div className="hidden w-full min-w-0 items-center justify-center sm:flex"> |
There was a problem hiding this comment.
P2: The workspace list search/filter is completely hidden below the sm breakpoint (hidden ... sm:flex), and no replacement is provided in the mobile layout. Mobile users lose the ability to search/filter their workspace list entirely.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/features/workspaces/components/WorkspaceHomePage.tsx, line 90:
<comment>The workspace list search/filter is completely hidden below the `sm` breakpoint (`hidden ... sm:flex`), and no replacement is provided in the mobile layout. Mobile users lose the ability to search/filter their workspace list entirely.</comment>
<file context>
@@ -87,8 +87,8 @@ function WorkspaceHomeNavbarControls({
return (
- <div className="flex w-full min-w-0 items-center justify-center">
- <div className="relative w-full min-w-0 max-w-56 sm:max-w-72">
+ <div className="hidden w-full min-w-0 items-center justify-center sm:flex">
+ <div className="relative w-full min-w-0 max-w-72">
<Search
</file context>
| const [status, setStatus] = useState<"loading" | "ready" | "error">("loading"); | ||
| const [isCaptureActive, setIsCaptureActive] = useState(false); | ||
| const enableFileCapture = useWorkspaceViewCapabilities().fileCapture; | ||
| const captureActive = enableFileCapture && isCaptureActive; |
There was a problem hiding this comment.
P3: Clear capture state when file capture becomes disabled; otherwise resizing back to desktop can unexpectedly restore an old hidden capture session.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/features/workspaces/components/WorkspaceImageViewer.tsx, line 57:
<comment>Clear capture state when file capture becomes disabled; otherwise resizing back to desktop can unexpectedly restore an old hidden capture session.</comment>
<file context>
@@ -52,16 +53,26 @@ function WorkspaceImageViewerContent({
const [status, setStatus] = useState<"loading" | "ready" | "error">("loading");
const [isCaptureActive, setIsCaptureActive] = useState(false);
+ const enableFileCapture = useWorkspaceViewCapabilities().fileCapture;
+ const captureActive = enableFileCapture && isCaptureActive;
+ const exitCapture = useCallback(() => {
+ setIsCaptureActive(false);
</file context>
Greptile SummaryThis PR adds a dedicated mobile workspace experience: a new
Confidence Score: 4/5Safe to merge; all three findings are minor style/polish items with no data-loss or broken-flow risk. The core branching logic (useSyncExternalStore, context provider placement, capability checks in each viewer) is solid and well-separated. The three comments are small: a div[role=button] that could be a real button, a missing overflow-hidden on the mobile file-viewer wrapper, and a misplaced dialog inside a header. None affect correctness of the mobile workspace flow. WorkspaceUploadClickTarget.tsx (accessibility element choice) and WorkspaceFileViewer.tsx (missing overflow containment on mobile path). Important Files Changed
Reviews (1): Last reviewed commit: "fix(workspaces): improve mobile item row..." | Re-trigger Greptile |
|
T-Rex pricing update — T-Rex was free through June 2026. Effective July 1, 2026, T-Rex adds 2 credits on top of the standard 1-credit review (3 total). T-Rex settings |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| return ( | ||
| <div | ||
| role="button" | ||
| tabIndex={0} | ||
| className={cn( | ||
| "cursor-pointer rounded-lg outline-none focus-visible:ring-3 focus-visible:ring-ring/50", | ||
| className, | ||
| )} | ||
| aria-label={ariaLabel} | ||
| onClick={onUploadFiles} | ||
| onKeyDown={handleKeyDown} | ||
| > | ||
| {children} | ||
| </div> | ||
| ); |
There was a problem hiding this comment.
div[role="button"] instead of a native <button> means the element misses built-in focus management, implicit form association, and the browser's native activation semantics (e.g. Space fires click on keyup for real buttons, not keydown). Replacing with <button type="button"> achieves the same result with better semantics and avoids the manual onKeyDown handler entirely.
| return ( | |
| <div | |
| role="button" | |
| tabIndex={0} | |
| className={cn( | |
| "cursor-pointer rounded-lg outline-none focus-visible:ring-3 focus-visible:ring-ring/50", | |
| className, | |
| )} | |
| aria-label={ariaLabel} | |
| onClick={onUploadFiles} | |
| onKeyDown={handleKeyDown} | |
| > | |
| {children} | |
| </div> | |
| ); | |
| return ( | |
| <button | |
| type="button" | |
| className={cn( | |
| "cursor-pointer rounded-lg outline-none focus-visible:ring-3 focus-visible:ring-ring/50", | |
| className, | |
| )} | |
| aria-label={ariaLabel} | |
| onClick={onUploadFiles} | |
| > | |
| {children} | |
| </button> | |
| ); |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| if (!viewCapabilities.contextMenus) { | ||
| return <div className="h-full min-h-0">{viewerContent}</div>; | ||
| } |
There was a problem hiding this comment.
viewerContent inside a ContextMenuTrigger rendered as section.overflow-hidden, which clamps any viewer overflow. The new mobile path wraps the same content in a plain div without overflow-hidden, so a misbehaving or oversized viewer (e.g. a PDF page wider than the viewport) could bleed out of its container on mobile.
| if (!viewCapabilities.contextMenus) { | |
| return <div className="h-full min-h-0">{viewerContent}</div>; | |
| } | |
| if (!viewCapabilities.contextMenus) { | |
| return <div className="h-full min-h-0 overflow-hidden">{viewerContent}</div>; | |
| } |
| <WorkspaceShareDialog | ||
| membershipRole={workspace.membershipRole} | ||
| onOpenChange={setShareOpen} | ||
| open={shareOpen} | ||
| workspaceId={workspace.id} | ||
| workspaceName={workspace.name} | ||
| /> | ||
| </header> |
There was a problem hiding this comment.
WorkspaceShareDialog is mounted inside the <header> element. Radix portals the dialog to document.body so it renders correctly, but semantically a dialog is not header content. Moving it to be a sibling of WorkspaceFrame (or just outside the <header>) would make the intent clearer and avoids lint rules or future tooling that validates landmark content.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Greptile SummaryThis PR adds a dedicated mobile workspace experience by introducing a
Confidence Score: 4/5The change is additive and the mobile path is well-isolated; desktop behavior is unchanged. The overall change is well-structured: mobile and desktop paths are cleanly separated, capability gates are consistently applied across all file viewers, and the new upload click target has correct accessibility attributes. Two issues are worth addressing before shipping: the hydration path in workspace-view-policy.tsx will produce a visible shell swap on mobile, and the content-center class on CardHeader in WorkspaceItemCard.tsx is a no-op on single-line flex columns. workspace-view-policy.tsx and WorkspaceItemCard.tsx warrant a quick look before merging. Important Files Changed
Reviews (2): Last reviewed commit: "fix(workspaces): improve mobile item row..." | Re-trigger Greptile |
| function getWorkspaceViewportServerSnapshot(): WorkspaceViewportMode { | ||
| return "desktop"; | ||
| } |
There was a problem hiding this comment.
getWorkspaceViewportServerSnapshot always returns "desktop", so any server-rendered (or static) page will initially mount the full desktop WorkspaceShell + WorkspaceSplitPresentation on a mobile device, then unmount it and mount WorkspaceMobileLayout after hydration fires. The swap is large enough (two different shell components) that users will see a visible layout flash and likely a reflow of the entire workspace area. Because useSyncExternalStore suppresses the mismatch warning, the issue is silent in development.
| </div> | ||
| ) : null} | ||
| <CardHeader className="pointer-events-none relative z-10 shrink-0 gap-1 px-3 py-2"> | ||
| <CardHeader className="pointer-events-none relative z-10 min-w-0 flex-1 content-center justify-start gap-1 py-2 pr-24 pl-3 sm:flex-none sm:shrink-0 sm:px-3"> |
There was a problem hiding this comment.
content-center does not vertically center items in a flex-col container
content-center maps to align-content: center, which only affects multi-line flex / grid containers. A single-column flex container (which CardHeader is) is never multi-line, so this rule is a no-op. On mobile, where the card uses flex-row and CardHeader takes flex-1, the title and metadata will be top-aligned instead of vertically centered. Use justify-center to center content along the main (column) axis.
| <CardHeader className="pointer-events-none relative z-10 min-w-0 flex-1 content-center justify-start gap-1 py-2 pr-24 pl-3 sm:flex-none sm:shrink-0 sm:px-3"> | |
| <CardHeader className="pointer-events-none relative z-10 min-w-0 flex-1 justify-center justify-start gap-1 py-2 pr-24 pl-3 sm:flex-none sm:shrink-0 sm:px-3"> |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| <WorkspaceShareDialog | ||
| membershipRole={workspace.membershipRole} | ||
| onOpenChange={setShareOpen} | ||
| open={shareOpen} | ||
| workspaceId={workspace.id} | ||
| workspaceName={workspace.name} | ||
| /> | ||
| </header> |
There was a problem hiding this comment.
WorkspaceShareDialog rendered inside <header>
Dialog portals render at document.body regardless of where they are declared, so this works visually. However, declaring a dialog inside a <header> is semantically unexpected and makes the ownership of the state (shareOpen) harder to locate during future refactors. Moving the dialog declaration to the root of WorkspaceMobileLayout (adjacent to the chat overlay) would make the structure easier to follow.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
1 participant
Summary
Why
Mobile users should browse a workspace or chat without cramped desktop chrome, and item rows should be easier to scan and tap. Desktop behavior remains intact, including thumbnails, context menus, and title rename affordances.
Changes
Testing
Review Notes
Start with WorkspaceLayout.tsx, WorkspaceMobileLayout.tsx, workspace-view-policy.tsx, and the item card chrome files.
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by CodeRabbit
New Features
Bug Fixes